Skip to content

fix(sentinel): split Umbrella identities into flat host and user fields - #105

Merged
ubercylon8 merged 1 commit into
mainfrom
fix/umbrella-identity-split
Aug 12, 2026
Merged

fix(sentinel): split Umbrella identities into flat host and user fields#105
ubercylon8 merged 1 commit into
mainfrom
fix/umbrella-identity-split

Conversation

@ubercylon8

@ubercylon8 ubercylon8 commented Aug 12, 2026

Copy link
Copy Markdown
Owner

Asked for a table of hostnames, a local model checked the Umbrella output, found no field named anything like "hostname", and went looking in Tenable and LimaCharlie instead. The hostname was in the rows it already had.

What the column actually holds

Identities_s is a two-element JSON array on 98% of dns rows (356,571 of 362,944 in a 1h sample):

element contains @ distinct what it is
[0] 0 739 the Anyconnect roaming client — a machine name
[1] 100% 724 the AD user (UPN)

matching the order of Identity_Types_s (["Anyconnect Roaming Client","AD Users"]).

Why this is our bug, not the model's

PR #104 made that column searchable but left it shaped as a JSON array inside a single flat evidence value. The findings schema is flat by contract precisely so a small model never has to parse a value to read it — and list_sentinel_incidents already learned this the hard way: live validation caught it returning Owner and the tactics inside AdditionalData as raw JSON strings, and both were parsed into readable evidence. The Umbrella surfaces never got the same treatment. So "who" was answerable and "which machine" was not.

The change

Rows now carry identity_host and identity_user as separate fields, replacing the raw array in place so field grouping survives. Identity_Types_s is consumed as the classifier rather than echoed back as another array to parse.

Classification is type-driven, not positional — the ordering is this connector's convention, not a guarantee. Fallbacks are deliberate:

  • types absent or misaligned → fall back to shape (@ means a user)
  • the ~1% of rows with a third AD Groups element → surfaced as identity_other, because calling a group a hostname would be the same class of error this split exists to remove
  • a truncated or non-JSON value → kept unparsed rather than dropped, since losing an identity is worse than showing it raw

Verification

Live: the seven blocked Tailscale hosts now report machine names (SBL8773, SB11199, SBL3878, …) alongside their users, in one call, with no raw array left in the output.

1080 tests passing (+9), ruff and mypy clean, no gen_docs drift. Five mutations run; one initially survived — it hit the malformed-JSON branch my tests didn't cover, so a test was added and it now fails correctly.

🤖 Generated with Claude Code

https://claude.ai/code/session_01XYTy7da8Z5ZHhkwcCpjojZ

@ubercylon8 @claude
fix(sentinel): split Umbrella identities into flat host and user fields
66787d4 
Asked for a table of hostnames, a local model checked the Umbrella output,
found no field named anything like "hostname", and went looking in Tenable and
LimaCharlie instead. The hostname was in the rows it already had.

`Identities_s` is a JSON array of two identities on 98% of dns rows (356,571 of
362,944 in 1h): element 0 the Anyconnect roaming client — a machine name, 739
distinct, none containing "@" — and element 1 the AD user, 724 distinct, all
UPNs, ordered to match Identity_Types_s. PR #104 made that column searchable but
left it shaped as a JSON array inside a single evidence value.

That shape is the defect. The findings schema is flat by contract precisely so a
small model never has to parse a value, and `list_sentinel_incidents` already
learned this lesson the same way — live validation caught it returning `Owner`
and the tactics inside `AdditionalData` as raw JSON strings, and both were
parsed into readable evidence. The Umbrella surfaces never got the same
treatment, so "who" was answerable and "which machine" was not.

Rows now carry `identity_host` and `identity_user` as separate fields, with
`Identity_Types_s` consumed as the classifier rather than echoed back as another
array to parse. Classification is driven by the type array, not by position: the
ordering is this connector's convention, not a guarantee. Where types are absent
or do not line up, it falls back to shape ("@" means a user). The ~1% of rows
carrying a third AD Groups element surface it as `identity_other` — calling a
group a hostname would be the same class of error this split removes — and a
truncated or non-JSON value is kept unparsed rather than dropped.

Verified live: the seven blocked Tailscale hosts now report machine names
(SBL8773, SB11199, SBL3878, …) alongside their users, in one call, with no raw
array left in the output.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XYTy7da8Z5ZHhkwcCpjojZ
@claude

claude Bot commented Aug 12, 2026 •
edited
Loading

Copy link
Copy Markdown

Claude finished @ubercylon8's task in 1m 43s —— View job

Security review — PR #105

  • Gather context (diff, changed files)
  • Review normalize.py identity-splitting logic
  • Review tools.py / server.py changes
  • Review test coverage
  • Check docs/skill updates for consistency
  • Post final findings

Verdict: no security or correctness issues found. This is a well-scoped normalization fix, consistent with CLAUDE.md's rules.

What I checked

  • Read-only / gating — no new tool surface, no write path touched. hunt_dns_web remains @guarded_tool("sentinel")-wrapped in server.py:78, and redact_finding() still runs over every finding in _render (server.py:27) before it leaves the server. Nothing in this diff bypasses that boundary.
  • Secrets/PII — identity_user surfaces the same AD UPN that was already present (just JSON-encoded) inside the old Identities_s evidence value pre-PR. This is a reshape, not a new exposure — no new PII class is introduced.
  • Findings schema flatness — the stated goal of the PR is met: _expand_identities (tools.py:182-205) replaces the raw JSON-array evidence value with flat identity_host / identity_user / identity_other keys, in place, so field grouping survives. This directly satisfies the "flat evidence" contract in CLAUDE.md's Critical Rule 4/5.
  • Injection risk — the only query-building change is adding a static column name (Identity_Types_s) to the project tuple in SURFACE_SPECS. It's not user-controlled and doesn't touch the indicator/KQL-building path (normalize.py:201 action_clause, or the indicator has clause), so no new injection surface.
  • Robustness of split_identities/_json_list (normalize.py:204-267):
    • Type-driven classification with a shape-based fallback (@ → user) is correct and tested for both matching and mismatched orderings.
    • Malformed/truncated JSON degrades to treating the raw string as a single value rather than raising or silently dropping data — good, and it's covered by test_split_survives_a_truncated_json_array / test_split_survives_a_value_that_is_not_json.
    • json.JSONDecodeError is a ValueError subclass, so it's caught by the existing except (ValueError, TypeError) — no unhandled exception path that could leak a raw traceback (which would otherwise need to go through the redaction boundary anyway via guarded_tool).
    • Row key ordering is preserved correctly since _expand_identities rebuilds the dict by iterating row.items() and substituting at the identity_field position — Python dict ordering guarantees this is deterministic.
  • Test coverage — good breadth: type-driven vs. positional fallback, third AD Groups element, missing types, non-JSON value, truncated JSON, empty value, and an end-to-end tools.py test asserting the raw Identities_s/Identity_Types_s columns are absent from final evidence (not just added-to). This directly targets the small-model-safe flat-schema contract, not just code coverage for its own sake.
  • Docs/skill sync — docs/reference/tools/sentinel.md and skills/sentinel/network-investigation/SKILL.md were updated consistently with the code change (no drift), addressing the exact failure mode described in the PR body (a model going to Tenable/LimaCharlie instead of using data already in hand).

No inline comments — didn't find anything rising to a reportable finding.

@ubercylon8
ubercylon8 merged commit bb67ead into main Aug 12, 2026
11 checks passed
@ubercylon8
ubercylon8 deleted the fix/umbrella-identity-split branch August 12, 2026 15:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@ubercylon8